Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

nixos/sshd: Remove algorithms that do MAC-then-encrypt #231165

Merged
merged 1 commit into from
May 11, 2023

Conversation

mweinelt
Copy link
Member

Algorithms with the -etm suffix calculate the MAC after encryption, which is generally considered safer.

Description of changes
Things done
  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandbox = true set in nix.conf? (See Nix manual)
  • Tested, as applicable:
  • Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
  • Tested basic functionality of all binary files (usually in ./result/bin/)
  • 23.05 Release Notes (or backporting 22.11 Release notes)
    • (Package updates) Added a release notes entry if the change is major or breaking
    • (Module updates) Added a release notes entry if the change is significant
    • (Module addition) Added a release notes entry if adding a new NixOS module
  • Fits CONTRIBUTING.md.

Algorithms with the -etm suffix calculate the MAC after encryption,
which is generally considered safer.
@mweinelt mweinelt changed the title nixos/sshd: Remove MACs that do encrypt-then-mac nixos/sshd: Remove algorithms that do MAC-then-encrypt May 10, 2023
@teto
Copy link
Member

teto commented May 10, 2023

does this need a changelog entry ? ie., are there some subtle implications (I can't think of any but still).

@mweinelt
Copy link
Member Author

ETM MACs were introduced in 6.2. This shouldn't cause any problems.

@teto teto merged commit 537d611 into NixOS:master May 11, 2023
@mweinelt mweinelt deleted the openssh-encryp-then-mac branch May 11, 2023 11:31
@chvp
Copy link
Member

chvp commented May 16, 2023

This breaks compatibility with all applications based on libssh2 (notably, libvlc).

@K900
Copy link
Contributor

K900 commented May 16, 2023

Upstream fix: libssh2/libssh2#987

Maybe we can backport this somehow?

@chvp
Copy link
Member

chvp commented May 16, 2023

Even if we backport this, other OS's/applications interacting with a NixOS sshd (which is my usecase) will stil remain broken. Adding a changelog entry as suggested above seems like a good idea at least.

@reedrw
Copy link
Contributor

reedrw commented May 16, 2023

does this need a changelog entry ? ie., are there some subtle implications (I can't think of any but still).

It breaks the "Run script over SSH" action in the iOS Shortcuts app, which I use quite frequently. I ended up needing to add the removed algorithms back in my config. So I think there should be a changelog entry.

@ivan
Copy link
Member

ivan commented May 20, 2023

This broke iOS PhotoSync with NixOS as its SFTP target; when uploading, it is stuck at 0% and reports nothing useful in the app; fortunately at least the server says

sshd[17331]: Unable to negotiate with 192.168.10.176 port 49419: no matching MAC found. Their offer: hmac-sha2-256,hmac-sha2-512,hmac-sha1,hmac-sha1-96,hmac-md5,hmac-md5-96,hmac-ripemd160,[email protected] [preauth]

@mweinelt
Copy link
Member Author

The changelog entry will look like this:

  • MAC-then-encrypt algorithms were removed from the default selection of services.openssh.settings.Macs. If you still require these MACs, for example when you are relying on libssh2 (e.g. VLC) or the SSH library shipped on the iPhone, you can re-add them like this:

    services.openssh.settings.Macs = [
      "hmac-sha2-512"
      "hmac-sha2-256"
      "[email protected]"
    };

tomfitzhenry added a commit to tomfitzhenry/nixpkgs that referenced this pull request Jun 5, 2024
Hardening SSH algorithms, which typically means dropping
all-but-the-strongest is of questionable value, given SSH's downgrade
protection[0]. We pay in compatibility, and maintenance.

Further, as noted in
https://github.com/NixOS/nixpkgs/pull/172393/files#r871727289 , both
the guidelines that we follow have not been updated in years.

The costs of having/maintaining these defaults:

* The burden of having a larger module that deviates from
upstream. We've slowly been reducing the upstream diff, to reduce
maintenance burden.
* Difficult for users to opt-out of these defaults. For example, when
using a "no OpenSSL" build of OpenSSH, having these defaults means
having to manually overriding NixOS's defaults. Upstream's defaults,
meanwhile, gracefully only use available algorithms, if OpenSSL is not
linked.
    * For users seeking to reduce attack surfaces that are fortunate
    enough to only have modern clients, they could choose to use
    `pkgs.opensshPackages.openssh.override { linkOpenssl = false; }`,
    which only supports chacha20-poly1305 and curve25519-sha256.
* NixOS#231165 unexpectedly broke some
clients.
* The time in discussing/reviewing these defaults.
* Anecdotally, a friend trying NixOS for the first time with a
ssh_config supporting only ecdh-* key exchanges was unable to SSH in
after install.

There's a certain level of enjoyment that comes from researching and
selecting a favourite suite of ciphers, but as a distro, it's not our
core competancy, and best left for upstream who are active in
advances/attacks/compatibility.

0. https://eprint.iacr.org/2016/072.pdf
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants